Skip to content

Conversation

@joefarebrother
Copy link
Contributor

Limits the size of the allowImplicitRead predicate to improve performance on databases with a large number of TupleElementContents.

@joefarebrother joefarebrother added the no-change-note-required This PR does not need a change note label Apr 16, 2025
Copilot AI review requested due to automatic review settings April 16, 2025 08:43
@joefarebrother joefarebrother requested a review from a team as a code owner April 16, 2025 08:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • python/ql/src/Variables/LoopVariableCapture/LoopVariableCaptureQuery.qll: Language not supported

isSink(node) and
(
cs instanceof DataFlow::TupleElementContent or
cs.(DataFlow::TupleElementContent).getIndex() in [0 .. 10] or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to do this, which doesn't restrict the index, is to properly implement ContentSet, and then have a branch of ContentSet represent all possible TupleElementContents, like e.g. Ruby has a branch that represent all possible element contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed; but it seems like it would be quite a significant task to properly implement ContentSet for python.

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Let's go with the quick fix for the time being. 👍

@joefarebrother joefarebrother merged commit 299b3f7 into github:main Apr 29, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants